fix boundary and performance issues in MathUtil conversions - #94
Merged
Conversation
safeFloatToInt accepted 2147483648f: widening Integer.MAX_VALUE to float rounds it up to 2^31, so the upper-bound check never tripped and the cast silently saturated to 2147483647. Compare as double instead. toBigInteger guarded a large negative scale but not a large positive one, so a tiny value such as 1E-10000000 reached BigDecimal.toBigInteger(), which computes 10^scale - several seconds of CPU to produce zero, and an undocumented ArithmeticException at larger exponents. Such values truncate to zero, so return that directly. Also add an explicit null check to toBigInteger, matching the other methods, and correct a few copy-pasted javadoc return types. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three issues in
MathUtil, each covered by a new test.safeFloatToIntaccepted2147483648f. WideningInteger.MAX_VALUEtofloatrounds it up to2^31, sof > Integer.MAX_VALUEwas false for exactly2^31and the(int)cast silently saturated to2147483647— the wrong answer from a method whose job is to reject out-of-range input. Comparing asdoublefixes it. The lower bound was already correct, since-2^31is exactly representable, andsafeDoubleToIntwas never affected.toBigIntegerguarded a large negative scale but not a large positive one. A value like1E-10000000hasintegerDigits == -9999999andscale == +10000000, so neither existing condition tripped andBigDecimal.toBigInteger()was reached. That callssetScale(0), which computes10^scale: measured at just over 4 seconds to return0from a 12-character input, and at larger exponents it throwsArithmeticException: BigInteger would overflow supported rangeinstead of the documentedIllegalArgumentException. The string passesparseAsBigDecimal's length limit easily, and the path is reachable from parsed content viaJavaDecimalHolder/XmlObjectBase.getBigIntegerValue(). Any such value truncates to zero, so returnBigInteger.ZEROdirectly — which also makes every sub-1 conversion cheaper.toBigIntegerhad no explicit null check, unlike every other method here. It already threwNullPointerExceptionfromstripTrailingZeros(), so this is consistency rather than a behaviour change;toLongandtoIntinherit it.Also added a comment on the existing
scale() < -DEFAULT_MAX_NUMBER_CHARScondition, which reads as redundant but is not: for1E+2147483647theprecision() - scale()subtraction overflows toInteger.MIN_VALUEand only the scale check catches it. And corrected a few copy-pasted javadoc@returntypes (parseAsDoubledocumentedfloat).Ran the
impl.utilandimpl.valuestests locally — all pass. Leaving the full suite to CI.🤖 Generated with Claude Code